[JExtract] Unify mechanisms between value types and reference types #240
Conversation
Unify memory and instance management mechanism between value types (e.g. `string` or `enum`) and reference types (e.g. `class` and `actor`). Now all imported nominal types are allocated using the value witness table, are returned indirectly, and are destroyed in the same manner. `SwiftInstance` is now the abstract base class for all the imported types including reference types. Concrete types can simply construct it by calling `super(memorySegment, arena)`.
| try (var arena = Arena.ofConfined()) { | ||
| // we need to make a pointer to the self pointer when calling witness table functions: | ||
| MemorySegment indirectDest = arena.allocate(SwiftValueLayout.SWIFT_POINTER); | ||
| MemorySegmentUtils.setSwiftPointerAddress(indirectDest, dest); | ||
| MemorySegment indirectSrc = arena.allocate(SwiftValueLayout.SWIFT_POINTER); | ||
| MemorySegmentUtils.setSwiftPointerAddress(indirectSrc, src); | ||
|
|
||
| var returnedDest = (MemorySegment) mh.invokeExact(indirectDest, indirectSrc, wtable); | ||
| return returnedDest; | ||
| try { | ||
| return (MemorySegment) mh.invokeExact(dest, src, wtable); |
There was a problem hiding this comment.
This initializeWithCopy change was not included in #236. But I realized this has the same issue with destroy. No need for extra indirection anymore.
| } | ||
|
|
||
| printer.printTypeDecl("public final class \(decl.javaClassName) implements \(parentProtocol)") { | ||
| printer.printTypeDecl("public final class \(decl.javaClassName) extends SwiftInstance implements \(parentProtocol)") { |
| \(decl.renderCommentSnippet ?? " *") | ||
| */ | ||
| public \(parentName.unqualifiedJavaTypeName)(\(renderJavaParamDecls(decl, paramPassingStyle: .wrapper))) { | ||
| this(/*arena=*/null, \(renderForwardJavaParams(decl, paramPassingStyle: .wrapper))); |
There was a problem hiding this comment.
I wonder if we should remove the constructors without an arena passed in actually… fine to keep in this pr, wdyt tho?
There was a problem hiding this comment.
I'm fine with removing this constructor as we don't want users to use ofAuto() arena as much as possible.
Also mixing different arena in a session sounds scary 😅 E.g.
try(var arena = SwiftArena.ofConfined()) {
var obj = new MyClass(arena);
var value = new MyStruct(); // Accidental use of "auto" arena.
obj.process(value); // Probably fine, but...
}This is actually probably fine, but still it's not ideal.
Removing this convenience constructor prevents this kind of issues.
There was a problem hiding this comment.
Yeah, it’ll be more consistent to just always pass arenas, might save us weird headaches down the line.
you can remove here or we’ll do later, your call
| // public SwiftAnyType(SwiftHeapObject object) { | ||
| // if (object.$layout().name().isEmpty()) { | ||
| // throw new IllegalArgumentException("SwiftHeapObject must have a mangled name in order to obtain its SwiftType."); | ||
| // } | ||
| // | ||
| // String mangledName = object.$layout().name().get(); | ||
| // var type = SwiftKit.getTypeByMangledNameInEnvironment(mangledName); |
There was a problem hiding this comment.
I don't feel we need this function anymore. Every instance has $swiftType(). I don't even think object.$layout().name() is a mangled name (i.e. it's initialized with .withName(SwiftKit.nameOfSwiftType(typeMetadata, true));)
There was a problem hiding this comment.
Yeah let's remove, it is from the time when we were still calling directly into initializers from the java side I think
Users should consitently use the same arena. Also since nothing was retaining the '.ofAuto()' arena was evil.
rintaro
force-pushed
the
jextract-value-or-reference
branch
from
91ca289 to
971bb92
Compare
Because that seems to be more common
| public \(parentName.unqualifiedJavaTypeName)(SwiftArena arena, \(renderJavaParamDecls(decl, paramPassingStyle: .wrapper))) { | ||
| var mh$ = \(descClassIdentifier).HANDLE; | ||
| try { | ||
| public \(parentName.unqualifiedJavaTypeName)(\(renderJavaParamDecls(decl, paramPassingStyle: .wrapper)), SwiftArena arena) { |
There was a problem hiding this comment.
I'm 90% sure that arena parameter at the end is more common practice.
Although I haven't been able to find any wrapper constructor examples,
There was a problem hiding this comment.
Last sounds ok me, makes the calls more similar to their swift equivalents and only the arena added on.
2 participants
Part of #236 with minimal
Sources/JExtractSwiftchanges. All the changes for it are going to be replaced with the remaining changes in #236 . This PR is mostly for reviewingSwiftKitchanges.Unify memory and instance management mechanism between value types (e.g.
stringorenum) and reference types (e.g.classandactor). Now all imported nominal types are allocated using the value witness table, are returned indirectly, and are destroyed in the same manner.SwiftInstanceis now the abstract base class for all the imported types including reference types. Concrete types can simply construct it by callingsuper(memorySegment, arena).